Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(RHINENG-9249): create bifrost toggle #2176

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

mkholjuraev
Copy link
Contributor

Resolves: https://issues.redhat.com/browse/RHINENG-9249

This creates a new toggle to switch between bootc table view and hybrid inventory tabs. This toggle should only be shown to accounts that has bootc images.

To test:

  1. Run the PR
  2. Open the app with an account that has bootc images (ping me for an account with bootc)
  3. Observe the new toggle is present and by default hybrid tabs are shown
  4. Click on the bootc toggle on the right
  5. Observe that you see a mock bootc table
  6. Open the app with another account that does not have any bootc image
  7. Observe that the toggle is not shown

@mkholjuraev mkholjuraev requested a review from a team as a code owner April 15, 2024 14:59
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 89.36170% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 57.40%. Comparing base (6ad8fe0) to head (ef09b32).

Files Patch % Lines
src/Utilities/edge.js 33.33% 2 Missing ⚠️
src/routes/InventoryComponents/BifrostTable.js 50.00% 1 Missing ⚠️
...c/routes/InventoryComponents/HybridInventory.cy.js 0.00% 1 Missing ⚠️
src/routes/InventoryComponents/HybridInventory.js 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2176      +/-   ##
==========================================
+ Coverage   57.26%   57.40%   +0.14%     
==========================================
  Files         199      203       +4     
  Lines        6295     6323      +28     
  Branches     1764     1767       +3     
==========================================
+ Hits         3605     3630      +25     
- Misses       2690     2693       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@computercamplove
Copy link
Contributor

/retest

@computercamplove
Copy link
Contributor

@mkholjuraev

  1. for what this feature flag hbi.ui.bifrost - when it was decided to use?

  2. since Systems page require filtering and this filtering works only with "hbi.api.xjoin-disable" is enabled for specific account - account without this feature flag gets 500 and page is not loading, i don't see check for this flag
    https://stage.foo.redhat.com:1337/api/inventory/v1/hosts?filter[system_profile][bootc_status][booted][image_digest][is]=not_nil&per_page=1
    image

  3. toggle works as expected in account that has image-mode systems and feature flag "hbi.api.xjoin-disable" is enabled

@mkholjuraev
Copy link
Contributor Author

@computercamplove to your questions:

  1. The frontend has an agreement to put a feature flag in every new feature
  2. I did not know since yesterday evening's meeting about this backend flag. As it is backend-related, should it be used in frontend???
  3. It was said yesterday that this flag was not in prod. I thought the backend getting rid of it, do I understand correctly that

@mkholjuraev mkholjuraev marked this pull request as draft April 16, 2024 08:57
@mkholjuraev mkholjuraev marked this pull request as draft April 16, 2024 08:57
@computercamplove
Copy link
Contributor

computercamplove commented Apr 16, 2024

@computercamplove to your questions:

1. The frontend has an agreement to put a feature flag in every new feature

2. I did not know since yesterday evening's meeting about this backend flag. As it is backend-related, should it be used in frontend???

3. It was said yesterday that this flag was not in prod. I thought the backend getting rid of it, do I understand correctly that
  1. thank you for explanation, i was asking because Bifrost team decided that they don't need UI flag check - user don't have image mode systems so they can't see all UI changes related to Bifrost (for context https://redhat-internal.slack.com/archives/C06EDRT4KB6/p1712666815175369 , https://redhat-internal.slack.com/archives/C02T90PLJ5V/p1711476987847739?thread_ts=1711475282.279549&cid=C02T90PLJ5V). But it's fine to have - in this case we will have enabled in both environments to keep automated tests running. If there will any issue we will disable it in production via your flag.
  2. i guess so, without this flag check filter url is not working on all accounts as you said yesterday (and as Dan explained this filter doesn't work with xjoin and works only when Xjoin is disabled - Xjoin disabled only for specific account, not on whole stage/prod, HBI team still works on migration), if you want we can clarify it again via thread with Chris/Dan.
  3. "hbi.api.xjoin-disabled" is on prod (i just need to add account number to it so we can test it later)

@computercamplove
Copy link
Contributor

here clarification
Filtering image-mode systems without Xjoin is expected to be ready by Summit
for context
https://redhat-internal.slack.com/archives/CQFKM031T/p1713273872452199

@johnsonm325
Copy link
Contributor

@mkholjuraev We'll need to handle errors. The api for checking for bootc returned a 500 and the page just loads and never errors
image

@mkholjuraev mkholjuraev marked this pull request as ready for review April 17, 2024 13:06
@mkholjuraev
Copy link
Contributor Author

@johnsonm325 it seems this continuous loading is because of this handler that only considers 403 errors. I think this was to solve some issues. @gkarat I am wondering if we can generalise that handler to other error types as well?

@gkarat
Copy link
Contributor

gkarat commented Apr 18, 2024

@mkholjuraev, yeah, the 403 handler was introduced here #2138, you can also find some details why it was done. You can for sure extend this block and treat other codes if it's necessary.

Copy link
Contributor

@johnsonm325 johnsonm325 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good enough to approve.

Just fix conflicts and address the small things I've pointed out.

Comment on lines 38 to 44
Inventory.defaultProps = {
initialLoading: true,
notificationProp: PropTypes.object,
};
Inventory.propTypes = {
isImmutableTabOpen: PropTypes.bool,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, will remove them

Comment on lines +32 to +33
const isBifrostEnabled = useFeatureFlag('hbi.api.disable-xjoin');
const { hasBootcImages } = useContext(AccountStatContext);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth it to provide isBifrostEnabled via the AccountStatContext as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context is used to hold information about an account. I believe the flag would not be a proper thing to add.

@mkholjuraev mkholjuraev merged commit 5fc6d2d into RedHatInsights:master Apr 19, 2024
2 checks passed
@gkarat
Copy link
Contributor

gkarat commented Apr 19, 2024

🎉 This PR is included in version 1.68.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants